Fix backslash escape, multiple linters output, .pyenv/versions folder search#920
Fix backslash escape, multiple linters output, .pyenv/versions folder search#920MikhailArkhipov merged 101 commits intomicrosoft:masterfrom MikhailArkhipov:master
Conversation
|
@MikhailArkhipov We're missing the following check in // Exclude files opened by vscode when showing a diff view.
if (uriSchemesToIgnore.indexOf(document.uri.scheme) >= 0) {
return;
}Here are the file schemes that need to be excluded (found in |
DonJayamanne
left a comment
DonJayamanne
left a comment
There was a problem hiding this comment.
Please fix #916 in this PR if possible (easy fix)
|
Sure. We probably want all checks from |
|
@MikhailArkhipov Yes, the issue numbers are linked in the message, but the title isn't and that's what I see in the GitHub notifications to determine if this PR is something I need to look at. |
Codecov Report
@@ Coverage Diff @@
## master #920 +/- ##
=========================================
- Coverage 64.34% 64.04% -0.3%
=========================================
Files 260 260
Lines 12042 12025 -17
Branches 2133 2131 -2
=========================================
- Hits 7748 7701 -47
- Misses 4285 4315 +30
Partials 9 9
Continue to review full report at Codecov.
|
|
Fixed #916 + tests. Need to watch one test stability on AppVeyor |
| this.documents.textDocuments.forEach(async document => { | ||
| public async lintOpenPythonFiles(): Promise<vscode.DiagnosticCollection> { | ||
| this.documents.textDocuments.forEach(document => { | ||
| if (document.languageId === PythonLanguage.language) { |
There was a problem hiding this comment.
This check is no longer necessary as you now have shouldLintDocument method that does the necessary checking. Besides this only checks one condition and could be misleading about the logic.
| } | ||
|
|
||
| public async lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<void> { | ||
| public async lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<vscode.DiagnosticCollection> { |
There was a problem hiding this comment.
This method doesn't feel right. Its modifying the underlying collection and returning the same thing.
Feels unnecessary to return the whole collection.
E.g. if you line a document, you'd expect to get errors related to that document, not all documents.
There was a problem hiding this comment.
Shoudn't the method public async lintOpenPythonFiles(): Promise<vscode.DiagnosticCollection> collect everything and send a consolidated view?
There was a problem hiding this comment.
This is for tests only, to get the active collection via vscode.commands.executeCommand('python.runLinting'). Currently linting of documents is completely non-blocking and I didn't want to wait until the entire collection is populated. However, it is doable.
|
OK, |
|
You can increase timeout for a single test as follows: test('Ensure non-existing files are not linted', async function () {
this.timeout(40000) |
|
Hmm, taking 35 seconds for linting, sounds something is wrong, that's an awfully long time. |
|
Local result: Local What is VM configuration on the test machine? RAM/Cores? |
|
Yay 👍 all tests pass |
| ui: 'tdd', | ||
| useColors: true, | ||
| timeout: 25000, | ||
| timeout: 35000, |
There was a problem hiding this comment.
Please could we increase the timeout of the individual test rather than doing this for all.
The problem with this approach is, it affects all tests.
This was necessary earlier, when most of the tests were running actual python code.
There was a problem hiding this comment.
Here's a sample:
test('Ensure non-existing files are not linted', async function () {
this.timeout(40000)
3 participants
Fixes #911 - Backslash escape for underscores in Markdown docstrings
Fixes #913 - Multiple linters not working as expected
Fixes search for
.pyenv/versionsfor #847.pyenv/versionsto make it into the absolute path